[SECUR-113] fix: ssrf for work item links #8607
Conversation
📝 WalkthroughWalkthroughAdds SSRF protections and strict redirect handling to URL fetching: enforces http/https schemes, resolves hostnames via getaddrinfo to validate all IPs against private/loopback/reserved/link-local ranges, introduces MAX_REDIRECTS with manual redirect loop, and applies validation when discovering and fetching favicons. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as Worker/Task
participant Validator as URL Validator
participant DNS as DNS Resolver
participant HTTP as HTTP Client
participant Remote as Remote Host
Worker->>Validator: validate initial URL (scheme+hostname)
Validator->>DNS: resolve hostname (getaddrinfo)
DNS-->>Validator: list of IPs
Validator->>Validator: check IPs vs private/loopback/reserved/link-local
alt allowed
Worker->>HTTP: HTTP request
HTTP->>Remote: connect/fetch
Remote-->>HTTP: response (200 or redirect)
HTTP-->>Worker: response
opt redirect
loop while redirects < MAX_REDIRECTS
Worker->>Validator: validate redirect URL
Validator->>DNS: resolve redirect hostname
DNS-->>Validator: IPs
Validator->>Worker: allowed?
Worker->>HTTP: follow redirect
end
end
else blocked
Validator-->>Worker: raise error (disallowed/unalottable)
end
Worker->>Validator: validate discovered favicon URL(s)
Worker->>HTTP: fetch favicon (if validated)
HTTP-->>Worker: favicon data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
sangeethailango
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/bgtasks/work_item_link_task.py (1)
27-63:⚠️ Potential issue | 🟠 MajorAddress DNS rebinding and multicast SSRF vectors.
The current validation has two security gaps: (1)
socket.getaddrinfo()resolves once here, butrequests.get()/requests.head()at lines 89 and later will resolve the hostname again independently—an attacker can rebind DNS between these calls to pivot to internal IPs; (2) multicast IPs (e.g.,224.0.0.1,ff02::1) are not blocked despite being unreachable from the public internet.Consider either pinning resolved addresses to the connection (via a custom adapter/resolver in requests) or validating the peer IP after connection. For the multicast gap, using
not ip.is_globalalone is insufficient since multicast addresses reportis_global=Trueand would still be allowed—you neednot ip.is_global or ip.is_multicast.
🤖 Fix all issues with AI agents
In `@apps/api/plane/bgtasks/work_item_link_task.py`:
- Around line 192-195: The SSRF check only validates the initial favicon_url but
requests.get(favicon_url) follows redirects, allowing a 302 to an internal IP to
bypass validation; update the fetch in work_item_link_task (where
validate_url_ip and requests.get are used) to either set allow_redirects=False
on requests.get or, if following redirects, re-validate the response.url with
validate_url_ip (same approach used in crawl_work_item_link_title_and_favicon)
before reading the body so redirected-to hosts cannot be internal/private.
🧹 Nitpick comments (1)
apps/api/plane/bgtasks/work_item_link_task.py (1)
146-161: Skip invalid favicon tags instead of aborting fallback.If a page exposes a
data:/blob:icon (or any non‑HTTP(S) icon),validate_url_ipraises and the function skips checking other tags and/favicon.ico. CatchingValueErrorlets you continue scanning and still fall back safely.♻️ Safer fallback behavior
for selector in favicon_selectors: favicon_tag = soup.select_one(selector) if favicon_tag and favicon_tag.get("href"): favicon_href = urljoin(base_url, favicon_tag["href"]) - validate_url_ip(favicon_href) - return favicon_href + try: + validate_url_ip(favicon_href) + except ValueError: + continue + return favicon_href @@ - except requests.RequestException as e: + except (ValueError, requests.RequestException) as e: log_exception(e, warning=True) return None
2 participants
Description
Fix SSR for work item links
Summary by CodeRabbit